Switch to PostgreSQL 16 image#577
Conversation
|
Extracted out of #395 |
|
Because #395 has both postgres and valkey in the same PR? |
|
I see the tests are failing on password encryption algorithm detection: https://github.com/theforeman/foremanctl/actions/runs/27768839212/job/82162895170?pr=577#step:17:643 Which is weird. |
f9dc89d to
f6f88a0
Compare
It's not weird, as that was the ext DB and that wasn't using our image definition properly. Fixed. |
| ansible.builtin.include_role: | ||
| name: debug_tools | ||
|
|
||
| - name: 'Enable postgresql:16 module' |
There was a problem hiding this comment.
@evgeni, let me ask this question again: wouldn't it be more resilient to run the PG commands from inside a PG container instead of installing the client tooling directly on the host?
There was a problem hiding this comment.
Would, sure. I just had none that I could integrate easily into the various workflows where we need Postgres tooling on the host.
There was a problem hiding this comment.
You'd likely want to write a module that does this to be re-used because there are postgres actions that are run in a lot more places. I am not sure I see the value of it at the moment but it might be worth exploring after we get everything else.
There was a problem hiding this comment.
@ehelms Debian 13 has PostgreSQL 17. That probably means you can't run pg_dump on the PostgreSQL 16 container we run, which breaks backups.
|
The upgrade complains: The fact its supporting 15→16 only is encoded here: Funnily, the container actually contains 13 binaries: I'd argue this is a bug in the image we should ask for a fix. |
72e983b to
7e0aadc
Compare
| - "../../../src/vars/defaults.yml" | ||
| - "../../../src/vars/flavors/{{ flavor }}.yml" | ||
| - "../../../src/vars/database.yml" | ||
| - "../../../src/vars/images.yml" |
| notify: | ||
| - Restart postgresql | ||
|
|
||
| - name: Check if PG_VERSION file exists |
There was a problem hiding this comment.
This set of upgrade tasks look good. Could they go in a dedicated task file?
| ansible.builtin.systemd: | ||
| name: postgresql.service | ||
| masked: false | ||
| when: |
There was a problem hiding this comment.
As this when pattern is repeated, you can put it in a block.
ekohl
left a comment
There was a problem hiding this comment.
Wouldn't it be easier to unconditionally set POSTGRESQL_UPGRADE so it will always run when needed?
Thinking about the logging aspect: where do the logs of the upgrade go now? Can we somehow debug it when it fails? When it's part of the regular service then you'll have it the journal and sosreport will pick it up already.
| POSTGRESQL_MAX_CONNECTIONS: "{{ postgresql_max_connections }}" | ||
| POSTGRESQL_SHARED_BUFFERS: "{{ postgresql_shared_buffers }}" | ||
| POSTGRESQL_EFFECTIVE_CACHE_SIZE: "{{ postgresql_effective_cache_size }}" | ||
| POSTGRESQL_PREV_VERSION: "{{ postgresql_version }}" |
There was a problem hiding this comment.
Please link why this workaround is needed
| POSTGRESQL_PREV_VERSION: "{{ postgresql_version }}" | |
| # https://github.com/sclorg/postgresql-container/issues/661 | |
| POSTGRESQL_PREV_VERSION: "{{ postgresql_version }}" |
| POSTGRESQL_SHARED_BUFFERS: "{{ postgresql_shared_buffers }}" | ||
| POSTGRESQL_EFFECTIVE_CACHE_SIZE: "{{ postgresql_effective_cache_size }}" | ||
| POSTGRESQL_PREV_VERSION: "{{ postgresql_version }}" | ||
| POSTGRESQL_UPGRADE: "hardlink" |
There was a problem hiding this comment.
Can you please make this a variable so we could easily switch to copy?
| - "postgresql_version == '13'" | ||
| register: postgresql_upgrade_result | ||
|
|
||
| - name: Stop PostgreSQL systemd service (Quadlet) |
There was a problem hiding this comment.
Why is it needed to stop the service again?
This will throw a warning: https://github.com/sclorg/postgresql-container/blob/134bb39ab5c435087487c3422522bddf160f727d/16/root/usr/share/container-scripts/postgresql/common.sh#L415 that will be visible in the logs (and if I am not mistaken it exits immediately, but at this point I am not sure if my environment was clean when I tested it). That was my original thought, but I had to work around the message.
Had to debug the process quite a lot. You will have limited logging in the playbook itself (I will remove the debug log level from the playbook), but the more interesting log will be in the data folder |
|
packit error seems unrelated: |
ekohl
left a comment
There was a problem hiding this comment.
This comment is not blocking, but something to consider. Possibly in a follow up.
| containers.podman.podman_container: | ||
| name: "{{ postgresql_container_name }}-upgrade" | ||
| image: postgresql.image | ||
| state: quadlet |
There was a problem hiding this comment.
Does it need to be a quadlet? In favor of it means you get logs in the journal, but it's also fragile: if Ansible somehow starts, it can leave the system in an unpredictable state.
There was a problem hiding this comment.
if Ansible somehow starts, it can leave the system in an unpredictable state.
I don't follow this logic. However, I assume you are asking the question of a quadlet which generates a systemd service vs. just running the container directly with the equivalent of a podman run?
There was a problem hiding this comment.
Does it need to be a quadlet?
I had the same question. I decided to go on with a quadlet to reuse the postgres.image container. If I spin it up as a separate container, it means I cannot reuse the image definition and any drop-ins that will be defined there. Originally it was a regular container.
There was a problem hiding this comment.
And I messed up: I intended to write "Ansible somehow stops in the middle". Not start.
There was a problem hiding this comment.
I don't think it is disastrous:
- if the playbook stops before defining the service - no harm done
- if it stops after the service is defined, but before it executes the service - it will be executed at first reboot
- if it stops after the execution - it may leave some leftovers, but they would be harmless
There are options to add the upgrade as a dependency for the postgres.container, so it will pull the service before the execution and then push the condition into the service startup, so it will just silently finish with noop in case the DB was already migrated.
I think it's too complex for a one time only step.
ekohl
left a comment
There was a problem hiding this comment.
I would be OK with tracking the items in a follow up task when someone actually does proper testing of upgrades. Right now I consider this "good enough" to make CI pass.
| quadlet_options: | ||
| - | | ||
| [Unit] | ||
| Description=PostgreSQL Database Upgrade from 13 to 16 |
There was a problem hiding this comment.
Perhaps make sure that systemd knows it's 1 or the other? Or will this introduce too much uncertainty?
| Description=PostgreSQL Database Upgrade from 13 to 16 | |
| Description=PostgreSQL Database Upgrade from 13 to 16 | |
| Conflicts=postgresql.service |
There was a problem hiding this comment.
According to Gemini, the directive is symmetric, so it won't protect us from someone that tries to start the server. Actually it will just cause the upgrade container to stop (even more problematic in the middle of an upgrade).
| Description=PostgreSQL Database Upgrade from 13 to 16 | ||
| [Service] | ||
| Type=oneshot | ||
| RemainAfterExit=yes |
There was a problem hiding this comment.
This means the service will remain as active. Is that useful?
There was a problem hiding this comment.
It will mark the service as active(exited) when it will stop the execution. In the happy path it won't matter, since we remove the service completely, but if it fails, we will have indication whether it was run or not.
Why are you introducing these changes? (Problem description, related links)
As part of being prepared to run on RHEL 10, we need to switch to postgres 16.
What are the changes introduced in this pull request?
How to test this pull request
If the system spins up successfully and works with content - it's good to go.